From 5db1316a6b60770867a09750f85ec31ce5b9da68 Mon Sep 17 00:00:00 2001 From: Huon Wilson Date: Tue, 18 Nov 2014 23:23:53 +1100 Subject: [PATCH] Warn about missing license/description in `cargo package`. It's very bad practice to not have a license in a published (theoretically) open-source package, since the default position is all-rights-reserved. Hence, cargo will now warn if this metadata field is missing from the manifest when creating a package. Similarly, a lack of description makes using crates.io less nice, since there's no indication of what a package does other than the name (and possibly documentation etc. links, but these are often missing too). These metadata fields are not immediately obvious so `cargo` can be a little intelligent and provide some hints that they exist. Closes #902. --- src/bin/package.rs | 5 ++- src/cargo/ops/cargo_package.rs | 41 +++++++++++++++++- src/cargo/ops/registry.rs | 5 ++- src/doc/manifest.md | 6 +++ tests/support/mod.rs | 1 + tests/test_cargo_package.rs | 77 +++++++++++++++++++++++++++++++++- tests/test_cargo_publish.rs | 6 +++ tests/test_cargo_registry.rs | 2 + 8 files changed, 138 insertions(+), 5 deletions(-) diff --git a/src/bin/package.rs b/src/bin/package.rs index ed981e742..057d5c90f 100644 --- a/src/bin/package.rs +++ b/src/bin/package.rs @@ -8,6 +8,7 @@ struct Options { flag_verbose: bool, flag_manifest_path: Option, flag_no_verify: bool, + flag_no_metadata: bool, flag_list: bool, } @@ -21,6 +22,7 @@ Options: -h, --help Print this message -l, --list Print files included in a package without making one --no-verify Don't verify the contents by building them + --no-metadata Ignore warnings about a lack of human-usable metadata --manifest-path PATH Path to the manifest to compile -v, --verbose Use verbose output @@ -31,7 +33,8 @@ pub fn execute(options: Options, shell: &mut MultiShell) -> CliResult let root = try!(find_root_manifest_for_cwd(options.flag_manifest_path)); ops::package(&root, shell, !options.flag_no_verify, - options.flag_list).map(|_| None).map_err(|err| { + options.flag_list, + !options.flag_no_metadata).map(|_| None).map_err(|err| { CliError::from_boxed(err, 101) }) } diff --git a/src/cargo/ops/cargo_package.rs b/src/cargo/ops/cargo_package.rs index 667b5a4eb..25bdfd714 100644 --- a/src/cargo/ops/cargo_package.rs +++ b/src/cargo/ops/cargo_package.rs @@ -26,11 +26,16 @@ impl Drop for Bomb { pub fn package(manifest_path: &Path, shell: &mut MultiShell, verify: bool, - list: bool) -> CargoResult> { + list: bool, + metadata: bool) -> CargoResult> { let mut src = try!(PathSource::for_path(&manifest_path.dir_path())); try!(src.update()); let pkg = try!(src.get_root_package()); + if metadata { + try!(check_metadata(&pkg, shell)); + } + if list { let root = pkg.get_manifest_path().dir_path(); let mut list: Vec<_> = try!(src.list_files(&pkg)).iter().map(|file| { @@ -62,6 +67,40 @@ pub fn package(manifest_path: &Path, Ok(Some(bomb.path.take().unwrap())) } +// check that the package has some piece of metadata that a human can +// use to tell what the package is about. +fn check_metadata(pkg: &Package, shell: &mut MultiShell) -> CargoResult<()> { + let md = pkg.get_manifest().get_metadata(); + + let mut missing = vec![]; + + macro_rules! lacking { + ($($field: ident),*) => {{ + $( + if md.$field.as_ref().map_or(true, |s| s.is_empty()) { + missing.push(stringify!($field)) + } + )* + }} + } + lacking!(description, license) + + if !missing.is_empty() { + let mut things = missing.slice_to(missing.len() - 1).connect(", "); + // things will be empty if and only if length == 1 (i.e. the only case to have no `or`). + if !things.is_empty() { + things.push_str(" or "); + } + things.push_str(*missing.last().unwrap()); + + try!(shell.warn( + format!("Warning: manifest has no {things}. \ + See http://doc.crates.io/manifest.html#package-metadata for more info.", + things = things).as_slice())) + } + Ok(()) +} + fn tar(pkg: &Package, src: &PathSource, shell: &mut MultiShell, dst: &Path) -> CargoResult<()> { diff --git a/src/cargo/ops/registry.rs b/src/cargo/ops/registry.rs index 02157c406..37bb22a91 100644 --- a/src/cargo/ops/registry.rs +++ b/src/cargo/ops/registry.rs @@ -32,8 +32,9 @@ pub fn publish(manifest_path: &Path, let (mut registry, reg_id) = try!(registry(shell, token, index)); try!(verify_dependencies(&pkg, ®_id)); - // Prepare a tarball - let tarball = try!(ops::package(manifest_path, shell, verify, false)).unwrap(); + // Prepare a tarball, with a non-surpressable warning if metadata + // is missing since this is being put online. + let tarball = try!(ops::package(manifest_path, shell, verify, false, true)).unwrap(); // Upload said tarball to the specified destination try!(shell.status("Uploading", pkg.get_package_id().to_string())); diff --git a/src/doc/manifest.md b/src/doc/manifest.md index 757183370..b98dbcf63 100644 --- a/src/doc/manifest.md +++ b/src/doc/manifest.md @@ -96,6 +96,12 @@ keywords = ["...", "..."] license = "..." ``` +The [crates.io](https://crates.io) registry will render the description, display +the license, link to the three URLs and categorize by the keywords. These keys +provide useful information to users of the registry and also influence the +search ranking of a crate. It is highly discouraged to omit everything in a +published crate. + # The `[dependencies.*]` Sections diff --git a/tests/support/mod.rs b/tests/support/mod.rs index ff61bb06f..36532ffdd 100644 --- a/tests/support/mod.rs +++ b/tests/support/mod.rs @@ -516,3 +516,4 @@ pub static PACKAGING: &'static str = " Packaging"; pub static DOWNLOADING: &'static str = " Downloading"; pub static UPLOADING: &'static str = " Uploading"; pub static VERIFYING: &'static str = " Verifying"; +pub static WARNING: &'static str = " Warning"; diff --git a/tests/test_cargo_package.rs b/tests/test_cargo_package.rs index c4a07ba27..4c6b68b4e 100644 --- a/tests/test_cargo_package.rs +++ b/tests/test_cargo_package.rs @@ -4,7 +4,7 @@ use tar::Archive; use flate2::reader::GzDecoder; use support::{project, execs, cargo_dir, ResultTest}; -use support::{PACKAGING, VERIFYING, COMPILING}; +use support::{PACKAGING, WARNING, VERIFYING, COMPILING}; use hamcrest::{assert_that, existing_file}; fn setup() { @@ -18,6 +18,8 @@ test!(simple { version = "0.0.1" authors = [] exclude = ["*.txt"] + license = "MIT" + description = "foo" "#) .file("src/main.rs", r#" fn main() { println!("hello"); } @@ -55,3 +57,76 @@ src[..]main.rs "unexpected filename: {}", f.filename()) } }) + +test!(metadata_warning { + let p = project("all") + .file("Cargo.toml", r#" + [project] + name = "foo" + version = "0.0.1" + authors = [] + "#) + .file("src/main.rs", r#" + fn main() {} + "#); + assert_that(p.cargo_process("package"), + execs().with_status(0).with_stdout(format!("\ +{packaging} foo v0.0.1 ({dir}) +{verifying} foo v0.0.1 ({dir}) +{compiling} foo v0.0.1 ({dir}[..]) +", + packaging = PACKAGING, + verifying = VERIFYING, + compiling = COMPILING, + dir = p.url()).as_slice()) + .with_stderr("Warning: manifest has no description or license. See \ + http://doc.crates.io/manifest.html#package-metadata for more info.")); + + let p = project("one") + .file("Cargo.toml", r#" + [project] + name = "foo" + version = "0.0.1" + authors = [] + license = "MIT" + "#) + .file("src/main.rs", r#" + fn main() {} + "#); + assert_that(p.cargo_process("package"), + execs().with_status(0).with_stdout(format!("\ +{packaging} foo v0.0.1 ({dir}) +{verifying} foo v0.0.1 ({dir}) +{compiling} foo v0.0.1 ({dir}[..]) +", + packaging = PACKAGING, + verifying = VERIFYING, + compiling = COMPILING, + dir = p.url()).as_slice()) + .with_stderr("Warning: manifest has no description. See \ + http://doc.crates.io/manifest.html#package-metadata for more info.")); + + let p = project("both") + .file("Cargo.toml", format!(r#" + [project] + name = "foo" + version = "0.0.1" + authors = [] + license = "MIT" + description = "foo" + "#)) + .file("src/main.rs", r#" + fn main() {} + "#); + assert_that(p.cargo_process("package"), + execs().with_status(0).with_stdout(format!("\ +{packaging} foo v0.0.1 ({dir}) +{verifying} foo v0.0.1 ({dir}) +{compiling} foo v0.0.1 ({dir}[..]) +", + packaging = PACKAGING, + verifying = VERIFYING, + compiling = COMPILING, + dir = p.url()).as_slice())); + +}) diff --git a/tests/test_cargo_publish.rs b/tests/test_cargo_publish.rs index 5d2d7f7bd..edd3d237e 100644 --- a/tests/test_cargo_publish.rs +++ b/tests/test_cargo_publish.rs @@ -41,6 +41,8 @@ test!(simple { name = "foo" version = "0.0.1" authors = [] + license = "MIT" + description = "foo" "#) .file("src/main.rs", "fn main() {}"); @@ -82,6 +84,8 @@ test!(git_deps { name = "foo" version = "0.0.1" authors = [] + license = "MIT" + description = "foo" [dependencies.foo] git = "git://path/to/nowhere" @@ -102,6 +106,8 @@ test!(path_dependency_no_version { name = "foo" version = "0.0.1" authors = [] + license = "MIT" + description = "foo" [dependencies.bar] path = "bar" diff --git a/tests/test_cargo_registry.rs b/tests/test_cargo_registry.rs index 5ae85d6f5..3eb5f982e 100644 --- a/tests/test_cargo_registry.rs +++ b/tests/test_cargo_registry.rs @@ -175,6 +175,8 @@ test!(package_with_path_deps { name = "foo" version = "0.0.1" authors = [] + license = "MIT" + description = "foo" [dependencies.notyet] version = "0.0.1" -- 2.30.2